fix(media): require authenticated reads by jmecom · Pull Request #4610 · block/buzz

jmecom · 2026-08-03T20:49:05Z

This change requires a valid signed Blossom authorization request and current relay membership for every media GET and HEAD request. It removes the unauthenticated compatibility path and updates desktop reads to send the required authorization.

This blocks anonymous retrieval and access after relay-membership revocation. It does not yet bind a blob to its originating channel, so someone removed from a private channel can still read a known blob while remaining a relay member. That channel-ACL follow-up remains required before closing the full finding.

Testing

git diff --check origin/main...codex/security-media-read-auth
Rebased onto origin/main at 5c98932
Full CI pending

Originating Buzz thread: buzz://message?channel=3928fe05-df61-4b5d-b9c7-d623b9b10ea1&id=3c6c02312f763fbe0d2bfc33a6c1a362f91d0354f3d18b039cf7a0558c1439d1

Make Blossom GET and HEAD require signed read authorization plus current relay membership unless an operator explicitly opts out. Co-authored-by: Jordan Mecom <jm@squareup.com> Signed-off-by: Jordan Mecom <jm@squareup.com>

Co-authored-by: Jordan Mecom <jm@squareup.com> Signed-off-by: Jordan Mecom <jm@squareup.com>

Media reads are now unconditionally authenticated, so BUZZ_REQUIRE_MEDIA_GET_AUTH no longer does anything. An operator who pinned it to `false` gets the stricter behaviour, which is correct, but silently — leaving them believing their deployment still serves media without auth. Warn at startup instead. BUZZ_REQUIRE_MEDIA_READ_AUTH is included because .env.example advertised it as an accepted alias while the relay never read it, so it may be set in existing configs. That stale line is gone now; note the removal in .env.example so operators grepping for either name find the answer. The lookup is injected so the check is testable without mutating process env, which is global and would race the other tests in this binary. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Signed-off-by: Eli Foster <efoster@squareup.com>

elifoster-block

Added a warning when config includes BUZZ_REQUIRE_MEDIA_GET_AUTH as it's no longer used.

wesbillman

Reviewing on Wes's behalf.

[P1] Update the runnable media acceptance contract before making read auth unconditional

This change correctly removes the rollout bypass, but it leaves the repository's advertised/manual media acceptance lane asserting the old unauthenticated behavior. Those tests are #[ignore], so required CI remains green while the lane is guaranteed to fail after this merge:

crates/buzz-test-client/tests/e2e_media.rs:147-183 sends bare successful GET, HEAD, and thumbnail requests.
crates/buzz-test-client/tests/e2e_media.rs:296-306 sends a bare missing-object GET and expects 404; auth now rejects it before storage lookup with 401.
crates/buzz-test-client/tests/e2e_media.rs:364-367 sends a bare real-image round-trip GET.
crates/buzz-test-client/tests/e2e_media_extended.rs:171-178,195-201 sends bare PNG/GIF round-trip GETs.
crates/buzz-test-client/tests/e2e_media_video.rs:273-278 sends a bare video GET.
crates/buzz-test-client/tests/e2e_media_video.rs:345-361,389-404 sends bare satisfiable and unsatisfiable range GETs, so neither the advertised 206 nor 416 behavior remains exercised after authentication.

The stale contract is also explicit outside those requests: crates/buzz-test-client/tests/conformance_multitenant.rs:2615-2626 says blob GET/HEAD stays public and unauthenticated, and docs/multi-tenant-conformance.md:52 repeats that requirement and leaves authenticated reads as an open question. This PR resolves that question in the opposite direction.

Please update successful GET/HEAD/thumbnail/video/range/missing-object cases to mint server- or hash-scoped kind-24242 t=get authorization, retain explicit bare-read 401 coverage, and rewrite the multi-tenant obligation/docs around authenticated host/tenant-scoped reads. Otherwise we remove the deployment escape hatch while silently breaking the executable and documented acceptance contract that is supposed to catch regressions in this boundary.

The implementation itself otherwise looks structurally sound from my route, tenant-binding, membership/revocation, and client-producer traces. The separately documented blob→channel ACL limitation remains deferred: relay membership plus a known hash is still sufficient even after private-channel removal.

Reads now require kind:24242 `t=get` auth, so every successful GET/HEAD/thumbnail/video/range/missing-object case in the media lane has to mint one. Add a hash-scoped `sign_blossom_get_auth` helper per test file and attach the header at each read site; the `x` tag matches on the sha256 before the extension, so one token covers `{sha}.jpg` and `{sha}.thumb.jpg` alike. Keep bare-read rejection explicit rather than implicit in the updated cases: `test_unauthenticated_reads_are_rejected` asserts 401 for a bare GET, HEAD and thumbnail GET. Rewrite the multi-tenant obligation and the conformance docs row around authenticated host/tenant-scoped reads, and drop the stale reference to the removed `require_media_get_auth` flag in the desktop persona card. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

elifoster-block · 2026-08-05T21:03:06Z

🤖 Posted by Eli's AI agent.

Addressed in e2ea443 and 171c360.

Read auth in the acceptance lane. Every successful read now mints a kind:24242 t=get event. Each of the three media test files gets a hash-scoped sign_blossom_get_auth helper; the x tag matches on the sha256 before the extension, so one token covers {sha}.jpg and {sha}.thumb.jpg alike. Sites updated:

e2e_media.rs — blob GET, HEAD, thumbnail GET, the missing-object GET in test_get_nonexistent_returns_404, and the real-image GET in test_upload_real_image.
e2e_media_extended.rs — PNG and GIF round-trip GETs.
e2e_media_video.rs — video blob GET plus both range GETs (the bytes=0-99 206 case and the out-of-range 416 case). Without auth the range behaviour under test is never reached.

The missing-object case is authenticated deliberately: auth runs before the storage lookup, so a bare request there would 401 rather than 404 and the test would stop asserting what it names.

Bare-read coverage retained explicitly rather than left implicit in the updated cases — test_unauthenticated_reads_are_rejected asserts 401 for a bare GET, bare HEAD and bare thumbnail GET.

Obligation and docs rewritten. media_metadata_boundary_holds_while_blob_bytes_shared and its pending_lane string now state that reads require host/hash-scoped Blossom auth bound to the header tenant, that bare reads 401 before storage, and that CAS bytes are still shared. The remaining blob→channel ACL gap is called out as a known, deferred limitation instead of being implied by the old public-read wording. Row 52 of docs/multi-tenant-conformance.md matches, including the client-compatibility note that read auth must now be presented (no flag restores the old behaviour). Also dropped a stale require_media_get_auth reference in desktop/src-tauri/src/commands/personas/card.rs.

The lane now actually runs in CI. Your point about #[ignore] keeping required CI green was worse than it looked: e2e_media, e2e_media_extended and e2e_media_video appeared nowhere in .github/ or scripts/. Backend Integration selects specific binaries with --run-ignored ignored-only (e2e_event_reminder, invite, relay_admin) and Relay E2E selects e2e_persona/e2e_team_catalog/e2e_nostr_interop/e2e_project/e2e_relay — the media binaries were in neither, so updating the tests alone would have left them unexecuted. 171c360 selects the lane in Relay E2E, which already starts a branch-built relay via start-relay-for-tests.sh with MinIO and the seeded localhost:3000 community:

      - name: Media read-auth e2e
        run: |
          cargo test -p buzz-test-client --test e2e_media --test e2e_media_extended --test e2e_media_video -- --ignored --nocapture
        env:
          RELAY_URL: ws://localhost:3000
          RELAY_HTTP_URL: http://localhost:3000

Verification status. clippy --all-targets and fmt --check are clean and cargo test -p buzz-media passes (107 tests, covering verify_blossom_get_auth scope matching). This is the first time the media lane executes anywhere, so its run may surface pre-existing failures unrelated to read auth; I'm watching it and will work through whatever it turns up. test_upload_real_image self-skips without TEST_IMAGE_PATH.

On the earlier push: all Desktop jobs passed including Desktop E2E Relay and Desktop Core; Desktop Smoke E2E (4) failed on thread-focus-mode.spec.ts:139 (toBeInViewport, viewport ratio 0), unrelated to this branch and green on the prior commit, so I reran that shard.

The `e2e_media`, `e2e_media_extended` and `e2e_media_video` binaries were `#[ignore]`d and selected by no job, so nothing verified that a real relay rejects bare reads or honours host- and hash-scoped `t=get` tokens. Select them in Relay E2E, which already has MinIO and the seeded 'localhost:3000' community. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

…lures The lane's first CI run failed `test_upload_png_roundtrip` with 422: the ffmpeg-generated fixture carries a pHYs chunk, and `validate_png_metadata_free` rejects pHYs as an identity channel. Drop the chunk (IHDR/IDAT/IEND only, still a decodable 2x2 RGB image) so the fixture matches the upload policy the relay actually enforces. Add --no-fail-fast to the lane: cargo stopped after e2e_media_extended failed, so e2e_media_video never reported at all. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

wesbillman

Reviewing on Wes's behalf at ddde9e954.

The previous acceptance-contract blocker is addressed. Successful blob, HEAD, thumbnail, missing-object, image, video, and range cases now mint hash-scoped kind-24242 t=get authorization; bare GET/HEAD/thumbnail requests retain explicit 401 coverage; the conformance obligation and operator documentation now describe authenticated host/tenant-scoped reads; and CI runs all three ignored media binaries with --no-fail-fast.

I also rechecked the server boundary with an independent reviewer: tenant resolution precedes auth; signatures, operation, freshness/expiration, and hash/host scope are verified; current community membership is checked on every read; and blob I/O remains behind auth and sidecar validation. We found no remaining bypass or compatibility blocker at this head. Required CI is green.

TESTING.md conflict: main added the BUZZ_DRAIN_JITTER_MS row where this branch deleted the BUZZ_REQUIRE_MEDIA_GET_AUTH row. Kept both intents — drain jitter documented, the now-inert media flag stays gone. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

wesbillman

Reviewing and approving on Wes's behalf at 0808f6bd0aa75ec924d3b5b7f76be8992967e59f.

The merge-conflict resolution preserves both intents: main's BUZZ_DRAIN_JITTER_MS documentation/config rows remain, while this branch's removal of the obsolete BUZZ_REQUIRE_MEDIA_GET_AUTH rows remains. The merge introduced no new behavior beyond that clean interleave, and the previously reviewed media-auth implementation and acceptance-contract fixes are unchanged.

The current CI run has one failing Desktop Smoke shard: inbox-edit.spec.ts times out waiting for the inbox composer attachment button on all three attempts; its artifact shows the inbox thread open with the composer rendered. That test and composer path are outside this PR's media-auth diff, while the PR-specific Rust lint, unit tests, security checks, and builds pass. I do not consider that unrelated desktop failure a blocker to this approval.

jmecom added 2 commits August 3, 2026 13:44

Require authentication for media reads by default

83feb2b

Make Blossom GET and HEAD require signed read authorization plus current relay membership unless an operator explicitly opts out. Co-authored-by: Jordan Mecom <jm@squareup.com> Signed-off-by: Jordan Mecom <jm@squareup.com>

Require authentication for all media reads

c43e99c

Co-authored-by: Jordan Mecom <jm@squareup.com> Signed-off-by: Jordan Mecom <jm@squareup.com>

jmecom marked this pull request as ready for review August 3, 2026 21:00

jmecom requested a review from a team as a code owner August 3, 2026 21:00

elifoster-block previously approved these changes Aug 4, 2026

View reviewed changes

wesbillman requested changes Aug 5, 2026

View reviewed changes

elifoster-block dismissed their stale review via e2ea443 August 5, 2026 21:02

elifoster-block and others added 2 commits August 5, 2026 14:38

elifoster-block requested a review from wesbillman August 5, 2026 22:32

wesbillman reviewed Aug 5, 2026

View reviewed changes

wesbillman previously approved these changes Aug 5, 2026

View reviewed changes

elifoster-block dismissed wesbillman’s stale review via 0808f6b August 5, 2026 23:10

wesbillman approved these changes Aug 5, 2026

View reviewed changes

elifoster-block enabled auto-merge (squash) August 5, 2026 23:37

Navigation Menu

Uh oh!

fix(media): require authenticated reads - #4610

fix(media): require authenticated reads#4610
jmecom wants to merge 7 commits into
mainfrom
codex/security-media-read-auth

jmecom commented Aug 3, 2026

Uh oh!

elifoster-block left a comment

Uh oh!

wesbillman left a comment

Uh oh!

elifoster-block commented Aug 5, 2026 •

edited

Loading

Uh oh!

wesbillman left a comment

Uh oh!

wesbillman left a comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Uh oh!

Conversation

jmecom commented Aug 3, 2026

Testing

Uh oh!

elifoster-block left a comment

Choose a reason for hiding this comment

Uh oh!

wesbillman left a comment

Choose a reason for hiding this comment

Uh oh!

elifoster-block commented Aug 5, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

wesbillman left a comment

Choose a reason for hiding this comment

Uh oh!

wesbillman left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

elifoster-block commented Aug 5, 2026 •

edited

Loading